r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Hmm. This seems a bit brittle. Why are we copying LLVM tools and the LLVM bitcode linker, but not wasm-component-ld, lld and other codegen backends? It feels like either the downloaded Rustc sysroot should already contain these things or we should add all of them to its sysroot unconditionally (if they are supposed to be built). Currently it looks like this is done rather arbitrarily.

Hmm. This seems a bit brittle. Why are we copying LLVM tools and the LLVM bitcode linker, but not wasm-component-ld, lld and other codegen backends? It feels like either the downloaded Rustc sysroot should already contain these things or we should add all of them to its sysroot unconditionally (if they are supposed to be built). Currently it looks like this is done rather arbitrarily.

wasm-component-ld and rust-lld are already included in the CI rustc and we can do the same for llvm-bitcode-linker too. But I am uncertain about adding LLVM tools since developers might prefer using custom LLVM builds.

I suppose that someone might want a custom/locally built LLD or WASM component build with CI rustc? 😆 And for working on custom codegen backends, it would also be useful to apply a custom backend to the downloaded sysroot, to avoid having to build the compiler.

Just to make sure that I understand the issue, I'll try to describe it: We build the rustc toolchain in CI with some options. Then, if you want to use the CI toolchain locally, your config.toml options should match what has been configured on CI. So if you set option rust.foo = true, then if it wasn't used on CI, you're out of luck, and the build fails. If it was configured on CI, we currently have the assumption that the option will be taken from the downloaded sysroot. On top of that, we have detection logic that tries to rebuild the sysroot if you have local modifications to compiler or library, thus again using the local version/build of that option.

This sort of seems to work for rust options, however there is an issue for llvm options. We currently don't check LLVM options at all in the download-ci-rustc machinery (AFAIK). We do check it for CI LLVM, and CI LLVM is required for CI rustc, so we kind of should know about the options present on CI. However, it's unclear whether the user wants to say "I want to use this (LLVM) option from CI" or "I want to build this (LLVM) option locally". I think that it would be great if we could let users configure thits (see below), but currently if they configure things that are not on LLVM CI, it will error the build out.

Currently, this decision was kind of arbitrary (we just copied the primary LLVM libs to the downloaded sysroots, because they are always needed, and just assumed that LLD will be there as a bonus). And this PR just changes that arbitrary logic to include a few more things.

I'm fine with doing this to unblock #122709, but it seems like just another hack, and we should think of some more general solution.

I would propose something like the following to fix the situation more generally:

1. Unify the logic of including stuff into local and downloaded sysroots (modulo some special requirements). So always include LLVM, LLD, tools, codegen backends, wasm-component-lld etc. into the built sysroot, regardless of where it comes from (of course, it will still depend on whether these options are configured in config.toml).
2. Since we require CI LLVM, these components will be taken from the CI LLVM sysroot by default. If they are not present there OR if there are local modifications to the source code of said tool (e.g. lld or wasm-component-ld), it should be rebuilt locally instead of being taken from CI LLVM.

What do you think?

EDIT: I realized that 2) is a bit more complex, because currently we just reject options incompatible with LLVM CI, rather than detecting local changes. But 1) seems like an improvement to the Assemble step.

This sort of seems to work for rust options, however there is an issue for llvm options. We currently don't check LLVM options at all in the download-ci-rustc machinery (AFAIK).

Yeah, we don't check [llvm] section options in the ci-rustc logic. As download-ci-llvm is required with download-rustc, we should never need that.

EDIT: I realized that 2) is a bit more complex, because currently we just reject options incompatible with LLVM CI, rather than detecting local changes. But 1) seems like an improvement to the Assemble step.

Agreed. Even the first one alone needs a lot of refactoring. For now, I've unified the three parts: llvm tools, llvm-bitcode-linker and wasm-component-ld. Once this PR is shipped, we can check the next day (with the new ci-rustc) to see if it’s enough (at least for now) for #122709.

Let's see what CI says.

@bors r+

📌 Commit b6fdb74 has been approved by Kobzol

It is now in the queue for this repository.

☔ The latest upstream changes (presumably #130133) made this pull request unmergeable. Please resolve the merge conflicts.

Rebased.

@bors r=Kobzol

📌 Commit 0e59075 has been approved by Kobzol

It is now in the queue for this repository.

@bors try

⌛ Trying commit 4967093 with merge e837dec...

☀️ Try build successful - checks-actions
Build commit: e837dec (e837decd8a8a0e3ab905aa38e17e865656e96ac3)

Fixed the problem of using incorrect build compiler in dist step.

@rustbot ready

@bors r+

📌 Commit 4967093 has been approved by Kobzol

It is now in the queue for this repository.

⌛ Testing commit 4967093 with merge adaff53...

☀️ Test successful - checks-actions
Approved by: Kobzol
Pushing adaff53 to master...

Finished benchmarking commit (adaff53): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 755.803s -> 756.349s (0.07%)
Artifact size: 341.20 MiB -> 341.21 MiB (0.00%)